-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Escape Content-Disposition params according to WHATWG HTML living standard #11571
Conversation
38e9e9a
to
d1e6d1a
Compare
d1e6d1a
to
8e787dd
Compare
value | ||
.replace("\"", "%22") | ||
.replace("\r", "%0D") | ||
.replace("\n", "%0A") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main part of this pull request, everything else just make use of this method and the rest is testing.
The link the the "WHATWG HTML living standard" section 4.10.21.8: https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#multipart-form-data
I updated the issues description now. Hopefully it's understandable, but just look at the code, I think it's pretty straight-forward. Just to make it clear, this pr is about when Play does generate the header, e.g. when it sends a request to some other service via ws. Parsing a multiplart/form data request from a client does work correctly, so most uses cases should be fine anyway. I am going to merge this since I worked on that a while ago where I was sure this is done, and now reviewed again carefully and I am very sure this is ok. Also actually this is an issue someone e-mailed me about and that person came up with a similiar patch (but that person did not handle all uses cases, only the main one), so that also confirms my approach here. |
@Mergifyio backport 2.8.x |
✅ Backports have been created
|
@mkurz Glad to see that it has been corrected successfully 👏 Content-Disposition filename escaping problem occurs in all https://gist.github.com/motoyasu-saburi/1b19ef18e96776fe90ba1b9f910fa714 I'm also very happy to be able to contribute to the Playframework in some small way. |
@motoyasu-saburi Thanks for your detailed report and proposed patch back then which helped to track down the problem! |
...TBA...So, what is this pull request about:
Currently, when Play needs to build a
Content-Disposition: form-data; name="..."; filename="..."
header, e.g. for sending mulitpart/form data via ws or when when using theRequestBuilder
, it does not escape the name or filename. That can cause problems, if a user-entered filename contains "malicious" letters. I wont go into details here, but it's clear that could cause security issues. However IMHO the impact shouldn't be to high in real world apps, because if a file gets uploaded to Play, Play will provide the file name escaped, retrieved from the multi part form body. If the file now gets forwarded to a backend service via ws it will likely be forwarded with the name escaped anyway. So to really make use of the vulnerability you need to set the name of the file from an external source e.g. from user input or from a database entry, etc. Still this can happen, so it's good to get that fixed ;)The patches I apply here follow the same approach that other frameworks use:
Offtopic:
What I find really funny, that because of the HTML5 strategy of escaping (file) names of "content-dispostion: form-data" headers, it is not possible for a server to find out if a user uploaded a file name named
hello" %22
, because the client (be it chrome, be it firefox or curl), will always encode"
to%22
, so the name gets transfered ashello%22 %22
, so when the server decodes the file name again it will think the file was calledhello" "
, so it's impossible to correctly transfer all information 😉After reading some RFC's and the HMLT5 standard regarging this encoding stuff, I don't know why the clients did now stick to the
*=
approach described in RFC5987, that Play e.g. uses for serving files.Some links, which I am just collecting here in case I have to come back to this issue and which helped me along the way: